test: Add custom function tests by pramodsatya · Pull Request #25480 · prestodb/presto

pramodsatya · 2025-07-02T22:30:57Z

Description

Adds e2e tests for Presto C++ custom functions. Testcases borrowed from AbstractTestQueries in presto-tests.

== NO RELEASE NOTE ==

czentgr · 2025-07-03T15:25:55Z

+
+ #include "presto_cpp/main/dynamic_registry/DynamicFunctionRegistrar.h"
+ #include "velox/expression/SimpleFunctionRegistry.h"
+ #include <iostream>


This goes to the top of the list.

czentgr · 2025-07-03T15:28:45Z

+ // (note the extern "C" directive to prevent the compiler from mangling the
+ // symbol name).
+
+namespace facebook::velox::common::dynamicRegistry {


Why this namespace? This is an example so the user could choose whatever namespace they have. I would change this away from a velox namespace and not use velox or presto namespace to indicate complete freedom.

Agree... we could use a namespace that is more representative of the current folder structure.

czentgr · 2025-07-03T15:29:44Z

+ #include "presto_cpp/main/dynamic_registry/DynamicFunctionRegistrar.h"
+ #include "velox/expression/SimpleFunctionRegistry.h"
+ #include <iostream>
+ // This file defines a mock function that will be dynamically linked and


This isn't technically a mock function. It is a real function that carries out an addition.

czentgr · 2025-07-03T15:32:20Z

+        return prestoRoot
+                .resolve("presto-native-execution")
+                .resolve("_build")
+                .resolve("release")


Can we change this an be able to check if release exists here and if not use debug? That would handle the case if the build type changes that you don't have control over here.

aditi-pandit

Thanks @pramodsatya. Have bunch of comments.

aditi-pandit · 2025-07-03T17:43:46Z

There shouldn't be a need to derive this class from one in presto-tests. We can make TestCustomFunctions a presto-native-tests only class. wdyt ?

Thanks for the suggestion, I agree.

aditi-pandit · 2025-07-03T17:46:50Z

This function should be a part of presto-native-tests and can be built by default. There isn't a need to put in the dynamic_registrations examples and set PRESTO_ENABLE_EXAMPLES for it. presto-native-execution needn't be changed at all.

Though you would need to create a top-level cpp code folder in presto-native-tests. So something like add them in a presto_cpp/tests/custom_functions folder.

Could we keep all C++ UDFs used in e2e tests in presto-native-execution/presto_cpp/main/dynamic_registry/examples, similar to how the Java test UDFs are all placed under presto-tests/src/main/java/com/facebook/presto/tests? Keeping them in presto-native-execution would ensure different dynamic libraries like .so and .dylib for Linux/Mac are suitably built during linux-build process in CI. Also the presto-native-tests CI workflow has a dependency on linux-build CI workflow, ensuring the dynamic libraries are built first. Otherwise, we could also leverage the container based testing framework for this? What do you suggest?

agree with @pramodsatya that having the linux-build process in CI as a dependency would build the libraries first. Also, keeping them under examples, for all other test files we come up with, users can take advantage of reading the code to these files if they stumble upon the dynamic registry function docs as well which could be advantageous.

if we were to build these in cpp in presto-native-tests, they will be built in presto-native-tests after the presto-native-execution build occurs in the linux-build workflow.

afaik, that should be fine if we can be sure that these dynamic files will be built before the test is run. @pramodsatya do you know if that can be made to be the case that the new cpp build for would happen before the E2E test run?

the other thing too, fmt::fmt gflags::gflags xsimd, would be dependencies for these libraries to build and we need to ensure that these will be available for the presto-native-tests cpp build.

So Java test udfs are in a testing specific directory... Its better to separate tests from examples. The examples are demo examples that are documentation. You can build the tests in the linux-build workflow if that is more convenient.

@pramodsatya @soumiiow : There is value to showing a custom function example that is not in presto-native-execution. Any user is more likely to copy that setup since they will write their functions in new java/cpp modules.

hey @aditi-pandit just to clarify, id make a Makefile, Cmakelists, put src files, etc and essentially build this completely under, say presto/presto-native-tests/presto-cpp/tests/custom_functions is that a correct interpretation of the ask?

Yes @soumiiow...

The build command for it can be in linux-build workflow if you prefer.

aditi-pandit · 2025-07-03T17:48:43Z

+ // (note the extern "C" directive to prevent the compiler from mangling the
+ // symbol name).
+
+namespace facebook::velox::common::dynamicRegistry {


Agree... we could use a namespace that is more representative of the current folder structure.

aditi-pandit · 2025-07-03T17:50:29Z

+} // namespace facebook::velox::common::dynamicRegistry
+
+extern "C" {
+    // In this case, we assume that facebook::presto::registerPrestoFunction


Don't follow this comment... registerPrestoFunction is available if you include the necessary header in this file.

aditi-pandit · 2025-07-03T17:50:58Z

+namespace facebook::velox::common::dynamicRegistry {
+
+    template <typename T>
+    struct DynamicFunctionCustomAdd {


This name can be simply CustomAdd

aditi-pandit · 2025-07-03T17:52:46Z

+    /// Custom aggregate functions are unsupported in Presto C++.
+    @Override
+    @Test (enabled = false)
+    public void testCustomSum() {}


These 2 test functions are not needed at all.

aditi-pandit · 2025-07-03T17:54:14Z

+    /// Sidecar is needed to support custom functions in Presto C++.
+    @Override
+    @Test
+    public void testCustomAdd()


As we are adding a new test for dynamic functions, we can add more scalar functions with a variety of simple types, array, map and struct based functions... nested types as well.

aditi-pandit · 2025-07-03T17:55:41Z

+
+    public static Path getCustomFunctionsPluginDirectory()
+    {
+        Path workingDir = Paths.get(System.getProperty("user.dir")).toAbsolutePath();


We should update the docs at https://github.com/prestodb/presto/tree/master/presto-native-tests#presto-native-tests as well for this config.

This path cannot be set in jvm arguments and its value is fixed, we intend to keep all the C++ UDFs used in e2e tests in presto-native-execution under the path presto_cpp/main/dynamic_registry/examples. Could you please confirm if this needs to be documented?

aditi-pandit · 2025-08-28T18:55:51Z

@pramodsatya & @mohsaka : Please can you merge your commits.

aditi-pandit

@mohsaka : This code is looking good minus the question about the test.

aditi-pandit · 2025-09-30T15:54:12Z

+    private boolean sidecarEnabled;
+
+    @BeforeSuite
+    public void buildNativeLibrary()


Am still not sure I follow : Why can we not do this in the pipeline ?

aditi-pandit · 2025-09-30T20:10:40Z

@mohsaka : There are velox update files in this PR. Do you need them or this was an accident ? I'll send an Advance Velox PR nonetheless.

mohsaka · 2025-09-30T20:25:06Z

@mohsaka : There are velox update files in this PR. Do you need them or this was an accident ? I'll send an Advance Velox PR nonetheless.

Sorry, did not intend to advance velox. Will remove it

aditi-pandit

Thanks @mohsaka. @czentgr PTAL as well.

Co-authored-by: Pramod Satya <pramod.satya@ibm.com> Co-authored-by: soumiiow <soumyaduriseti@gmail.com>

aditi-pandit

Thanks @mohsaka

pramodsatya requested review from a team, czentgr, elharo, feilong-liu, jaystarshot, unidevel and vivek-bharathan as code owners July 2, 2025 22:30

prestodb-ci added the from:IBM PR from IBM label Jul 2, 2025

prestodb-ci requested review from a team and sh-shamsan and removed request for a team July 2, 2025 22:31

pramodsatya requested review from aditi-pandit, soumiiow and tdcmeehan July 2, 2025 22:32

pramodsatya mentioned this pull request Jul 2, 2025

test(native): Add AbstractTestQueries to native tests #23671

Open

pramodsatya force-pushed the custom_func_native_tests branch from 8a0fcf9 to e2dc5bc Compare July 2, 2025 22:37

czentgr reviewed Jul 3, 2025

View reviewed changes

aditi-pandit reviewed Jul 3, 2025

View reviewed changes

soumiiow force-pushed the custom_func_native_tests branch from e2dc5bc to ce68951 Compare July 16, 2025 23:38

mohsaka force-pushed the custom_func_native_tests branch 3 times, most recently from 56fee7c to d7df27e Compare August 13, 2025 20:46

mohsaka self-assigned this Aug 22, 2025

mohsaka force-pushed the custom_func_native_tests branch 5 times, most recently from 574bacf to 471ebcc Compare August 28, 2025 03:14

mohsaka dismissed czentgr’s stale review via d9fcc4c September 23, 2025 00:55

mohsaka force-pushed the custom_func_native_tests branch from 28892b2 to d9fcc4c Compare September 23, 2025 00:55

mohsaka requested a review from aditi-pandit September 23, 2025 00:57

mohsaka force-pushed the custom_func_native_tests branch 8 times, most recently from e2ff78e to ff0d683 Compare September 27, 2025 01:02

aditi-pandit reviewed Sep 30, 2025

View reviewed changes

mohsaka force-pushed the custom_func_native_tests branch from ff0d683 to 36273b1 Compare September 30, 2025 18:27

mohsaka force-pushed the custom_func_native_tests branch from 36273b1 to 45b9801 Compare September 30, 2025 20:27

aditi-pandit previously approved these changes Sep 30, 2025

View reviewed changes

mohsaka dismissed aditi-pandit’s stale review via 6633845 September 30, 2025 22:20

mohsaka force-pushed the custom_func_native_tests branch 2 times, most recently from 6633845 to 72b0506 Compare October 1, 2025 17:27

Add E2E testing for Dynamic Functions.

fb09bb5

Co-authored-by: Pramod Satya <pramod.satya@ibm.com> Co-authored-by: soumiiow <soumyaduriseti@gmail.com>

mohsaka force-pushed the custom_func_native_tests branch from 72b0506 to fb09bb5 Compare October 1, 2025 20:54

mohsaka changed the title ~~[native] Add custom function tests~~ test: Add custom function tests Oct 2, 2025

aditi-pandit approved these changes Oct 2, 2025

View reviewed changes

czentgr approved these changes Oct 3, 2025

View reviewed changes

unidevel mentioned this pull request Nov 14, 2025

Add release notes for 0.296 unix280/presto#47

Merged

15 tasks

This was referenced Nov 24, 2025

Add release notes for 0.296 unix280/presto#48

Merged

Add release notes for 0.296 unix280/presto#49

Merged

unidevel mentioned this pull request Dec 1, 2025

Add release notes for 0.296 unix280/presto#50

Merged

prestodb-ci mentioned this pull request Dec 1, 2025

fix(docs): Add release notes for 0.296 #26726

Merged

19 tasks

Conversation

pramodsatya commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit commented Aug 28, 2025

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit commented Sep 30, 2025

Uh oh!

mohsaka commented Sep 30, 2025

Uh oh!

aditi-pandit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pramodsatya commented Jul 2, 2025 •

edited

Loading

aditi-pandit left a comment •

edited

Loading